API-9331 evaluator rewrite to handle more complex expressions#77
Merged
wlmcewen merged 5 commits intosparkapi:masterfrom Aug 7, 2025
Merged
API-9331 evaluator rewrite to handle more complex expressions#77wlmcewen merged 5 commits intosparkapi:masterfrom
wlmcewen merged 5 commits intosparkapi:masterfrom
Conversation
wlmcewen
commented
Aug 6, 2025
| assert !sample("Test Eq true Not (Test Eq true Or Test Eq false)") | ||
| assert !sample("Test Eq true Not (Not Test Eq false)") | ||
| assert !sample("Test Eq false And Test Eq true Not Test Eq false") | ||
| assert !sample("Test Eq true Not (Test Eq false Or Test Eq false) And (Test Eq false Or Test Eq false)") |
Contributor
Author
There was a problem hiding this comment.
This is the primary test we've added here.
codygustafson
approved these changes
Aug 7, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This reproduces a bug reported multiple times over the years in sparkql evaluation. We held off on a fix as it was clear the implementation needed a rewrite, and would be trivial in the v2 AST implementation we'd like to go in.
Well, that v2 branch is getting stale, and this bug has popped up again on CUR-29245, so I opted to split the difference in a rewrite that's closer to how a tree solution would operate.
Processing the expressions in the same block group is fairly trivial in the old implementation. The nested expressions is where things get tricky in the old implementation. Therefore, this implementation breaks up and groups a filter into expressions of the same block group.
From there, I process each block group starting with the most deeply nested groups. As we go back up the nesting levels, the block group is reduced to a result and a conjunction, which slides in like a placeholder for and expression. Thus the block group becomes a faux expression in the block level above it.
An AST approach would still be superior here, but the performance differences should be negligible.
Example:
Final thoughts: we got away with an ugly implementation for a long time, as the typical saved searches were fairly basic. As our UI and such started supporting more complex queries, we need to keep up.